refactor: Eliminate double call to Vmm::stop() AND fix(vcpu): Don't treat KVM_EXIT_{SHUTDOWN,HLT} as successful termination #5611
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5611 +/- ##
=======================================
Coverage 83.22% 83.22%
=======================================
Files 277 277
Lines 29279 29275 -4
=======================================
- Hits 24367 24364 -3
+ Misses 4912 4911 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ilstam
force-pushed
the
vm-shutdown
branch
2 times, most recently
from
50a24be to
043098e
Compare
JackThomson2
added
the
Status: Awaiting review
bchalios
left a comment
bchalios
left a comment
There was a problem hiding this comment.
Generally LGTM and thanks for the dive deep and nice clarification of the various exit codes and potential code paths. Just a comment regarding a comment :). Also: in your second commit you mention:
On x86 the guest asks for a CPU reset via the i8042 controller which Firecracker intercepts and kills the VM
I'm not sure this is entirely correct. IIUC, it's the VMM that raises an interrupt via the i8042 to the guest, then the guest somehow starts the process of resetting which ends up being a KVM exit handled by VMM again.
On x86 a CPU RESET is requested via an i8042 command. Firecracker intercepts this and writes to an eventfd which starts the termination process:
Are you referring to this FC API? https://github.com/firecracker-microvm/firecracker/blob/main/docs/api_requests/actions.md#intel-and-amd-only-sendctrlaltdel This simply injects a CtrlAltDel sequence to the guest. The guest is free to interpret this however it wants or ignore it. Depending on the way it's configured, Linux might start a soft reboot and eventually it will request a CPU reset via the i8042 command discussed above. There's no dedicated KVM exit (other than the MMIO exit that happens when the guest accesses the i8042). |
So, is it the case that event when we don't send the Ctrl+Alt+Delete, guest uses the i8042 device to reset? (This is just for my education. The PR looks good). |
Yes. If you type 'reboot' in the guest it will (eventually) ask for a CPU reset via the i8042. If you inject a Ctrl+Alt+Delete sequence it might ignore it or it might interpret it as if you typed 'reboot' inside the guest. If you type 'poweroff' I think it will try to do it via ACPI, but since we don't emulate ACPI power management the guest just hangs but the VM remains alive (presumably we re-enter the vCPUs but the guest runs an infinite loop). |
Commit 79c5f79 ("Shutdown VCPU threads so they can thread::join()") introduced a Vmm::stop() function that should run in the main VMM thread when a vCPU thread writes to the vcpus_exit_evt eventfd. Vmm::stop() sends a termination event to the vCPU threads and joins them. That commit claims that there was a cyclical dependency where vCPU objects referenced the Vmm objects and therefore Vmm::stop() could not be called from Vmm:drop(). Later, commit 1d45fcc ("vmm: fix drop logic for Vmm") introduced a second call to Vmm::stop(), this time from Vmm::drop(). As a result, when killing the VMM today (by issuing a reboot in the guest) Vmm::stop() is called twice and the "Vmm is stopping" message appears twice in the firecracker logs. Additionally, the documentation in Vmm:stop() is incorrect. Not all teardown paths call vcpu.exit(). In fact, the most common teardown path which is the guest asking for a CPU reset via the i8042 controller writes to the corresponding eventfd directly and vcpu.exit() is never called. Today, the vCPU threads do not hold references to the Vmm object and therefore it's fine to join the vCPU threads in Vmm::drop(). Eliminate the double call to Vmm::stop() (and the duplicate log message) and move the vCPU thread termination and join logic to Vmm::drop(). Vmm::stop() now simply sets Vmm::shutdown_exit_code which causes the VMM to break out of its main event loop and start the termination process. Additionally, remove part of the documentation that was already incorrect or becomes incorrect after this change. Signed-off-by: Ilias Stamatis <[email protected]>
This is almost a pure revert of commit 3a9a1ac ("exit with success code on certain KVM_EXIT events") which added code that treats KVM_EXIT_SHUTDOWN and KVM_EXIT_HLT as successful VM terminations. KVM_EXIT_SHUTDOWN is an exit code that KVM uses when an x86 CPU triple faults. KVM_EXIT_HLT is the exit code that KVM uses when the guest executes a HALT x86 instruction and KVM doesn't emulate the irqchip. Since we're using the in-kernel irqchip we should never see a KVM_EXIT_HLT exit, as HALT instructions are emulated by KVM and do not cause userspace exits. Do not return Ok(VcpuEmulation::Stopped) for these exit types since that ends up propagating an FcExitCode::Ok code to the main thread, even though these are abnormal terminations (especially the triple-fault one). Remove special handling for these x86-specific exit reasons and treat them as any other unexpected exit reason. Also, replace an incorrect comment that says that vCPUs exit with KVM_EXIT_SHUTDOWN or KVM_EXIT_HLT when the guest issues a reboot. On x86 the guest asks for a CPU reset via the i8042 controller which Firecracker intercepts and kills the VM. On ARM KVM exits to userspace with the reason KVM_EXIT_SYSTEM_EVENT (which we already handle correctly). Since we're in the neighbourhood get rid of a stale (8 year old) TODO comment questioning whether we should kill the VM when we get an unexpected KVM exit reason. Terminating the VM as we have been doing is completely reasonable. Fixes: 3a9a1ac ("exit with success code on certain KVM_EXIT events") Signed-off-by: Ilias Stamatis <[email protected]>
4 participants
Changes
These patches address the following in the VM shutdown path:
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.